Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid UnresolvedException in CurrentBatchTimestamp #23660

Closed
wants to merge 1 commit into from
Closed

[SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid UnresolvedException in CurrentBatchTimestamp #23660

wants to merge 1 commit into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Jan 26, 2019 •
edited
Loading

What changes were proposed in this pull request?

Spark replaces CurrentTimestamp with CurrentBatchTimestamp.
However, CurrentBatchTimestamp is TimeZoneAwareExpression while CurrentTimestamp isn't.
Without TimeZoneId, CurrentBatchTimestamp becomes unresolved and raises UnresolvedException.

Since CurrentDate is TimeZoneAwareExpression, there is no problem with CurrentDate.

This PR reverts the previous patch on MicroBatchExecution and fixes the root cause.

How was this patch tested?

Pass the Jenkins with the updated test cases.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid xception in CurrentBatchTimestamp [SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid UnresolvedException in CurrentBatchTimestamp Jan 26, 2019
dongjoon-hyun
dongjoon-hyun commented Jan 26, 2019
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/MicroBatchExecution.scala
CurrentBatchTimestamp(offsetSeqMetadata.batchTimestampMs,
ct.dataType)
ct.dataType, Some("Dummy TimeZoneId"))
case cd: CurrentDate =>
CurrentBatchTimestamp(offsetSeqMetadata.batchTimestampMs,
cd.dataType, cd.timeZoneId)
}

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following lines (511 ~ 519) are just a revert of the previous patch.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jan 26, 2019 •
edited
Loading

cc @HeartSaVioR , @jose-torres , @tdas , @zsxwing , @gaborgsomogyi , @arunmahadevan

dongjoon-hyun
dongjoon-hyun commented Jan 26, 2019
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/MicroBatchExecution.scala
CurrentBatchTimestamp(offsetSeqMetadata.batchTimestampMs,
ct.dataType)
ct.dataType, Some("Dummy TimeZoneId"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may try to use DateTimeUtils.defaultTimeZone().getID() or conf.sessionLocalTimeZone (as ResolveTimeZone does for TimeZoneAwareExpression).

However, IncrementalExecution doesn't use timezone info in case of CurrentBatchTimestamp(_, TimestampType, _). And, we want to prevent to use TimeZone for this CurrentTimestamp expression because this is originally non-TimeZoneAwareExpression.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing nice analysis and looks like this patch is simpler and more concise. Nice!

@SparkQA
Copy link

SparkQA commented Jan 26, 2019

Test build #101708 has finished for PR 23660 at commit fcbfcd5.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

Retest this please.

@SparkQA
Copy link

SparkQA commented Jan 26, 2019

Test build #101710 has finished for PR 23660 at commit fcbfcd5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

HeartSaVioR
HeartSaVioR reviewed Jan 26, 2019
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala
val df = input.toDS()
.withColumn("cur_timestamp", lit(current_timestamp()))
.withColumn("cur_date", lit(current_date()))
val df = input.toDS().withColumn("cur_timestamp", lit(current_timestamp()))
Copy link
Contributor

@HeartSaVioR HeartSaVioR Jan 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, adding cur_date was the reason why UT is passed even without the patch.

I added only current_timestamp() first (checked UT failed without the patch) and added current_date() afterwards, which looks like making cur_timestamp be resolved without any of patches.
(Though I'm not sure about the mechanism why it happens...)

Nice finding!

HeartSaVioR
HeartSaVioR approved these changes Jan 26, 2019
View reviewed changes
Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Happy to see cleaner patch with proper explanation of comments.

@HeartSaVioR HeartSaVioR mentioned this pull request Jan 26, 2019
Closed
@dongjoon-hyun
Copy link
Member Author

Thank you for review, @HeartSaVioR .

@dongjoon-hyun
Copy link
Member Author

Hi, @gatorsmile . Could you review this PR, too?

@gatorsmile
Copy link
Member

cc @jose-torres @zsxwing

@dongjoon-hyun
Copy link
Member Author

Thank you for approval, @srowen . Merged to master/branch-2.4.

dongjoon-hyun added a commit that referenced this pull request Jan 27, 2019
@dongjoon-hyun
[SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid UnresolvedE…
448a063 
…xception in CurrentBatchTimestamp

## What changes were proposed in this pull request?

Spark replaces `CurrentTimestamp` with `CurrentBatchTimestamp`.
However, `CurrentBatchTimestamp` is `TimeZoneAwareExpression` while `CurrentTimestamp` isn't.
Without TimeZoneId, `CurrentBatchTimestamp` becomes unresolved and raises `UnresolvedException`.

Since `CurrentDate` is `TimeZoneAwareExpression`, there is no problem with `CurrentDate`.

This PR reverts the [previous patch](#23609) on `MicroBatchExecution` and fixes the root cause.

## How was this patch tested?

Pass the Jenkins with the updated test cases.

Closes #23660 from dongjoon-hyun/SPARK-26379.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 1ca6b8b)
Signed-off-by: Dongjoon Hyun <[email protected]>
HeartSaVioR pushed a commit to HeartSaVioR/spark that referenced this pull request Jan 27, 2019
@dongjoon-hyun @HeartSaVioR
[SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid UnresolvedE…
52ac627 
…xception in CurrentBatchTimestamp

## What changes were proposed in this pull request?

Spark replaces `CurrentTimestamp` with `CurrentBatchTimestamp`.
However, `CurrentBatchTimestamp` is `TimeZoneAwareExpression` while `CurrentTimestamp` isn't.
Without TimeZoneId, `CurrentBatchTimestamp` becomes unresolved and raises `UnresolvedException`.

Since `CurrentDate` is `TimeZoneAwareExpression`, there is no problem with `CurrentDate`.

This PR reverts the [previous patch](apache#23609) on `MicroBatchExecution` and fixes the root cause.

## How was this patch tested?

Pass the Jenkins with the updated test cases.

Closes apache#23660 from dongjoon-hyun/SPARK-26379.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@dongjoon-hyun @jackylee-ch
[SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid UnresolvedE…
b2e814c 
…xception in CurrentBatchTimestamp

## What changes were proposed in this pull request?

Spark replaces `CurrentTimestamp` with `CurrentBatchTimestamp`.
However, `CurrentBatchTimestamp` is `TimeZoneAwareExpression` while `CurrentTimestamp` isn't.
Without TimeZoneId, `CurrentBatchTimestamp` becomes unresolved and raises `UnresolvedException`.

Since `CurrentDate` is `TimeZoneAwareExpression`, there is no problem with `CurrentDate`.

This PR reverts the [previous patch](apache#23609) on `MicroBatchExecution` and fixes the root cause.

## How was this patch tested?

Pass the Jenkins with the updated test cases.

Closes apache#23660 from dongjoon-hyun/SPARK-26379.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Jul 23, 2019
@dongjoon-hyun @kai-chi
[SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid UnresolvedE…
c950f97 
…xception in CurrentBatchTimestamp

## What changes were proposed in this pull request?

Spark replaces `CurrentTimestamp` with `CurrentBatchTimestamp`.
However, `CurrentBatchTimestamp` is `TimeZoneAwareExpression` while `CurrentTimestamp` isn't.
Without TimeZoneId, `CurrentBatchTimestamp` becomes unresolved and raises `UnresolvedException`.

Since `CurrentDate` is `TimeZoneAwareExpression`, there is no problem with `CurrentDate`.

This PR reverts the [previous patch](apache#23609) on `MicroBatchExecution` and fixes the root cause.

## How was this patch tested?

Pass the Jenkins with the updated test cases.

Closes apache#23660 from dongjoon-hyun/SPARK-26379.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 1ca6b8b)
Signed-off-by: Dongjoon Hyun <[email protected]>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Jul 25, 2019
@dongjoon-hyun @kai-chi
[SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid UnresolvedE…
5942f37 
…xception in CurrentBatchTimestamp

## What changes were proposed in this pull request?

Spark replaces `CurrentTimestamp` with `CurrentBatchTimestamp`.
However, `CurrentBatchTimestamp` is `TimeZoneAwareExpression` while `CurrentTimestamp` isn't.
Without TimeZoneId, `CurrentBatchTimestamp` becomes unresolved and raises `UnresolvedException`.

Since `CurrentDate` is `TimeZoneAwareExpression`, there is no problem with `CurrentDate`.

This PR reverts the [previous patch](apache#23609) on `MicroBatchExecution` and fixes the root cause.

## How was this patch tested?

Pass the Jenkins with the updated test cases.

Closes apache#23660 from dongjoon-hyun/SPARK-26379.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 1ca6b8b)
Signed-off-by: Dongjoon Hyun <[email protected]>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Aug 1, 2019
@dongjoon-hyun @kai-chi
[SPARK-26379][SS][FOLLOWUP] Use dummy TimeZoneId to avoid UnresolvedE…
223d173 
…xception in CurrentBatchTimestamp

## What changes were proposed in this pull request?

Spark replaces `CurrentTimestamp` with `CurrentBatchTimestamp`.
However, `CurrentBatchTimestamp` is `TimeZoneAwareExpression` while `CurrentTimestamp` isn't.
Without TimeZoneId, `CurrentBatchTimestamp` becomes unresolved and raises `UnresolvedException`.

Since `CurrentDate` is `TimeZoneAwareExpression`, there is no problem with `CurrentDate`.

This PR reverts the [previous patch](apache#23609) on `MicroBatchExecution` and fixes the root cause.

## How was this patch tested?

Pass the Jenkins with the updated test cases.

Closes apache#23660 from dongjoon-hyun/SPARK-26379.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 1ca6b8b)
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srowen srowen srowen approved these changes

@HeartSaVioR HeartSaVioR HeartSaVioR approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dongjoon-hyun @SparkQA @gatorsmile @srowen @HeartSaVioR